Fix interface bind #4510
Fix interface bind #4510
Conversation
alexpyattaev
force-pushed
the
fix_interface_bind
branch
from
4d5b1d9 to
bb1de96
Compare
|
It would be helpful as a reviewer to make this into a few distinct PRs (or commits that could be reviewed serially):
|
| pub fn get_public_ip_addr( | ||
| ip_echo_server_addr: &SocketAddr, | ||
| bind_address: Option<IpAddr>, | ||
| ) -> anyhow::Result<IpAddr> { | ||
| let fut = ip_echo_server_request( | ||
| *ip_echo_server_addr, | ||
| IpEchoServerMessage::default(), | ||
| bind_address, | ||
| ); | ||
| let rt = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build()?; | ||
| let resp = rt.block_on(fut)?; | ||
| Ok(resp.address) | ||
| } | ||
|
|
||
| pub fn get_cluster_shred_version(ip_echo_server_addr: &SocketAddr) -> Result<u16, String> { | ||
| let resp = ip_echo_server_request(ip_echo_server_addr, IpEchoServerMessage::default())?; | ||
| pub fn get_cluster_shred_version( | ||
| ip_echo_server_addr: &SocketAddr, | ||
| bind_address: Option<IpAddr>, | ||
| ) -> anyhow::Result<u16> { | ||
| let fut = ip_echo_server_request( | ||
| *ip_echo_server_addr, | ||
| IpEchoServerMessage::default(), | ||
| bind_address, | ||
| ); | ||
| let rt = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build()?; | ||
| let resp = rt.block_on(fut)?; | ||
| resp.shred_version | ||
| .ok_or_else(|| String::from("IP echo server does not return a shred-version")) | ||
| .ok_or_else(|| anyhow!("IP echo server does not return a shred-version")) | ||
| } |
There was a problem hiding this comment.
i suggest you deprecate these functions first so it doesn't break downstream users of net-utils. Steps: deprecate them, rewrite the new functions with the new bind_address option, change code to use the new functions
There was a problem hiding this comment.
Okay. I figured noone outside of anza is using these anyway. But if you think we must then we can do the whole deprecation dance as well.
There was a problem hiding this comment.
ya to be honest, I do not know if anyone is using these outside Anza. But if they are, good to not break the interface.
There was a problem hiding this comment.
yeah, it's sometimes hard to say: https://crates.io/crates/solana-net-utils
we've been slapped on the wrist for breaking public interfaces too often, so we've been getting stricter on this.
If we need to break compatibility or have a really compelling reason to, we can always ask Jacob/Nick to understand the downstream impact
There was a problem hiding this comment.
I figure we can keep old functions as they are, and add new ones that bind to interface. We do not always need to bind, so old ones will still be used.
| let rt = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build() | ||
| .expect("Can not create a runtime"); |
There was a problem hiding this comment.
.expect() error messages should be styled as "expect as precondition". see here: https://doc.rust-lang.org/std/error/index.html#common-message-styles. Basically formatted as "tokio builder should create a runtime" or something
| pub fn get_public_ip_addr( | ||
| ip_echo_server_addr: &SocketAddr, | ||
| bind_address: Option<IpAddr>, | ||
| ) -> anyhow::Result<IpAddr> { |
There was a problem hiding this comment.
would maybe just recommend staying with io::Result instead of changing to anyhow::Result.
There was a problem hiding this comment.
The original code did not report specific errors (it reported just String) so I have stuck with the convention. io::Result can not encode all ways this stuff can fail anyways, at least not cleanly. Since we are not handling these errors with match I see no point making life difficult.
I will close this PR and do the following three instead:
|
Problem
There is a bunch of code in net-utils that does not obey bind-address CLI arg when called from agave-validator.
This is part of a bigger problem as outlined in #4440
Summary of Changes
Fixes #